-
Notifications
You must be signed in to change notification settings - Fork 206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add LinesIter
algorithm to iterate over lines in a geometry
#757
Conversation
// └───────────────────────────────────────┘ | ||
|
||
type MultiLineStringIter<'a, T> = | ||
iter::Flatten<MapLinesIter<'a, slice::Iter<'a, LineString<T>>, LineString<T>>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would actually like to introduce newtypes for all these concrete LinesIter::Iter
types because they currently leak implementation details (about how iterators are chained etc).
I followed the pattern in CoordsIter
for this PR, but if there is agreement, I'm happy to introduce newtypes for both CoordsIter
and LinesIter
.
// Can't use LineString::lines() because it returns an `impl Trait` | ||
// and there is no way to name that type in `LinesIter::Iter` until [RFC 2071] is stabilized. | ||
// | ||
// [RFC 2071]: https://rust-lang.github.io/rfcs/2071-impl-trait-existential-types.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am new(ish) to Rust, and I spent some time trying to wrap my head around this.
My current understanding is that as long as RFC-2071 is unstable, crate APIs should likely not use the impl Trait
feature (in the return position, for existential types). Two reasons that I think I'd avoid this feature:
- Clients have no way to store the returned value in a struct / trait associated type (anywhere they must name the type).
- Conditional implementations (
#[derive(Debug)]
and friends) don't work well). In our case,impl Iterator<Item=Line<T>
is notDebug
even if bothT
andLine
are).
Basing my understanding so far mostly off of https://www.reddit.com/r/rust/comments/8ik620/notes_on_impl_trait/ and https://users.rust-lang.org/t/embedding-trait-inside-of-struct/37176/14
In this case, I was stuck because of the first issue -- I needed to name the type for LinesIter::Iter
, but had no way to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did I say I'm new to Rust? Please let me know if I'm understanding this wrong / I'm missing something obvious in how impl Trait
works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: Update #681 in some form if impl Trait
should be reconsidered in the public API of geo-types
.
@@ -2,8 +2,10 @@ | |||
|
|||
## Unreleased | |||
|
|||
* BREAKING: Make `Rect::to_lines` return lines in winding order for `Rect::to_polygon`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was probably oversight. Good catch!
I personally don't think this is the kind of change we'd bump semantic versioning for. Does anyone disagree?
I'm ok to leave "BREAKING" or whatever in the release notes, I'm just loathe to do anything that causes all our downstream crate users to be on an old version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any context on downstream consumers yet, so I don't have a clear opinion here.
I think this falls in the "Possibly-breaking change" per cargo book and in unstable packages, it's OK to bump just the patch version for these changes. The order of returned lines isn't part of an explicit contract, so someone depending on that order to not conform to the winding order is unlikely.
Let me know if I can help make this decision (perhaps start a thread on the channel?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the interest of a certain amount of efficiency, I don't expect to get an explicit "OK" on all things. Typically when I'm about to do something potentially controversial, my approach is to make a proposal in the code review and solicit input. If no one complains (we're all async, so I wait a couple days) I go forward.
If it were something that seemed potentially very controversial, I might wait for longer or for a more explicit OK from a larger group of people.
Since no one has yet replied to the contrary, I estimate that it's reasonable to move forward without bumping major version at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code all LGTM, but I want to leave it unmerged until someone else weighs in on not bumping semver for this change.
bors r+ |
Build succeeded: |
Thank you for a great first PR experience! |
CHANGES.md
if knowledge of this change could be valuable to users.Newly introduced
LinesIter
is very similar toCoordsIter
, but is implemented for only a subset of types (e.g., not forPoint
, and hence not forGeometry
).This PR also includes a small tweak to the order in which
Rect::to_lines()
returns the lines -- it now matches the winding order of the polygon created byRect::to_polygon()
.Closes #708